Actionable comments posted: 4

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@commands/gsd/plan-review-convergence.md`:
- Line 4: Update the argument-hint string to include the missing documented
flags so it's aligned with the CLI docs: change the existing argument-hint value
(the "argument-hint" entry in this file) from "<phase> [--codex] [--gemini]
[--all] [--max-cycles N]" to include the additional options (for example
"<phase> [--codex] [--gemini] [--claude] [--opencode] [--text] [--ws] [--all]
[--max-cycles N]"), and apply the same edit to the other argument-hint
occurrences noted elsewhere in the file (the repeated argument-hint blocks
around lines referenced in the comment).

In `@get-shit-done/workflows/plan-review-convergence.md`:
- Line 63: The markdown contains multiple fenced code blocks using ``` without
language tags which trips MD040; update each triple-backtick fence shown in the
diff (and the additional occurrences noted in the review) to include an
appropriate language identifier (for example use ```text for plain output
blocks, ```bash for shell snippets, or ```json for JSON examples) so every
fenced block is annotated and markdownlint MD040 is satisfied.
- Around line 138-140: The HIGH_COUNT assignment can produce "0\n0" because grep
-c exits nonzero when no matches and your || echo "0" runs again; change the
HIGH_COUNT pipeline to avoid emitting a second "0" and ensure a single numeric
value (for example, run grep -c ... 2>/dev/null || true to suppress the non-zero
exit, then normalize with parameter expansion like HIGH_COUNT=${HIGH_COUNT:-0});
update the assignment that defines HIGH_COUNT (and leave HIGH_LINES as-is) so
numeric comparisons against HIGH_COUNT work correctly.
- Around line 20-25: The script calls INIT via the node gsd-tools.cjs init
plan-phase command before extracting PHASE from ARGUMENTS, causing phase
metadata (phase_dir, padded_phase, has_plans, plan_count, etc.) to be
initialized with an empty/stale phase; reorder the logic so you first parse and
validate PHASE and other flags from ARGUMENTS (extract PHASE, ensure it's
non-empty, collect REVIEWER_FLAGS, MAX_CYCLES, GSD_WS, text_mode,
response_language), then run INIT=$(node
"$HOME/.claude/get-shit-done/bin/gsd-tools.cjs" init plan-phase "$PHASE") and
parse its JSON output into phase_dir, phase_number, padded_phase, phase_name,
has_plans, plan_count, commit_docs, text_mode, response_language; add explicit
validation to fail early if PHASE is empty and ensure response_language is
handled after parsing.

Adversarial review.

New command and workflow for a cross-AI plan convergence loop. Overall well-structured. Key findings:

Issues addressed by author: All 4 CodeRabbit findings (grep -c fallback, init-before-parse ordering, argument-hint completeness, code block language identifiers) confirmed addressed in commit c401fd3.

No security issues found: No command injection vectors, no path traversal, no unsafe eval/exec patterns. Agent spawning uses Skill() calls, not raw shell.

Structural concerns:

1. No test for the HIGH-count stall detection — the test suite covers loop structure, escalation gates, and REVIEWS.md verification, but there's no test verifying that prev_high_count == HIGH_COUNT triggers the stall warning. This is the most fragile logic path and the one most likely to silently regress.

2. --ws forwarding to review agent — the workflow spawns Skill('gsd-review', ...) without threading GSD_WS through to it. If a user is working in a named workspace, the review agent may read from the wrong workspace. This was flagged by CodeRabbit (id 3097651140) and the author addressed init ordering but it's not clear --ws is forwarded. Worth confirming.

3. Max-cycles=1 edge case — with --max-cycles 1, if the initial review finds HIGHs, the loop escalates immediately without a replan. The workflow text handles this but the test suite doesn't cover it.

Adversarial review — REQUEST CHANGES.

Trek-e's prior review (2026-04-17T13:15:29Z) raised three concerns. The author addressed CodeRabbit's four findings in commit c401fd3, but trek-e's structural concerns were not addressed. The PR has no response from the author on those points and no subsequent commits that cover them.

Unresolved concerns:

1. --ws forwarding to review agent (correctness bug)
The convergence workflow spawns Skill('gsd-review', args='--phase {PHASE} {REVIEWER_FLAGS}') but does not thread GSD_WS through to the review agent. If the user is working in a named workspace (--ws project-name), the review agent reads from the wrong workspace context. The replan agent does pass {GSD_WS} — making the behavior asymmetric: planning is workspace-aware, reviewing is not. The review agent must also receive the workspace flag.

2. Missing stall detection test
The test suite has 7 suites covering command structure, init, planning gate, convergence loop, stall detection, escalation gate, and artifact verification. The stall detection suite tests that prev_high_count is tracked and that a warning is emitted — but no test verifies the condition HIGH_COUNT >= prev_high_count triggers the warning when the count does not decrease between cycles. The existing stall tests are structural (string presence) rather than behavioral. This is the most fragile path: a refactor of the stall variable names or comparison would break stall detection silently with no test catching it.

3. --max-cycles 1 edge case
With --max-cycles 1, a cycle 1 review that finds HIGHs immediately hits the escalation gate without a replan attempt. The workflow handles this correctly in prose, but the test suite has no coverage for this boundary. Users who set --max-cycles 1 as a dry-run/review-only mode will hit behavior that is untested.

Required fixes before approval:

• Thread GSD_WS through to the review agent spawn call in plan-review-convergence.md
• Add a behavioral stall detection test: simulate decreasing HIGH count (no stall warning) and stable HIGH count (stall warning emitted)
• Add a --max-cycles 1 test covering the immediate escalation path

The spec, overall structure, and orchestrator-only design are correct. These are targeted gaps, not architectural problems.

Actionable comments posted: 1

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@get-shit-done/workflows/plan-review-convergence.md`:
- Around line 157-166: The workflow lacks a concrete parser for the agent's
return message; implement extraction logic that reads the agent output
(AGENT_RETURN), captures HIGH_COUNT by matching the CYCLE_SUMMARY line (regex
for current_high=\d+), validates it is a non-empty integer and aborts with the
exact message "Review agent did not honor the CYCLE_SUMMARY contract — cannot
determine HIGH count. Retry or switch reviewer." if missing or invalid, and
extracts HIGH_LINES as the bullet lines under the "## Current HIGH Concerns"
section (stop at the next "##" or end of message), treating a section that
contains only "None." as HIGH_LINES=""; ensure robustness to minor formatting by
trimming whitespace and handling missing sections consistently.

---

Outside diff comments:
In `@get-shit-done/workflows/plan-review-convergence.md`:
- Line 274: The success criteria line stating "grep HIGHs" is outdated; update
the Orchestrator success criteria to reflect that it should parse the review
agent's CYCLE_SUMMARY contract (see CYCLE_SUMMARY parsing logic referenced
around lines 157-166) to extract HIGH counts instead of grepping REVIEWS.md, and
clarify that the Orchestrator only handles init, loop control, stall detection,
extraction of HIGH counts from CYCLE_SUMMARY, and escalation.

---

Nitpick comments:
In `@get-shit-done/workflows/plan-review-convergence.md`:
- Around line 163-164: Add a validation step after extracting
CYCLE_SUMMARY/HIGH_COUNT that also verifies the "## Current HIGH Concerns"
section was present and parsed into HIGH_LINES (or explicitly contains only
"None."); if the section is missing or malformed (i.e., HIGH_LINES undefined or
not a string/empty when not "None."), abort with a clear error similar to the
CYCLE_SUMMARY check (e.g., "Review agent did not honor the CYCLE_SUMMARY
contract — cannot determine HIGH concerns. Retry or switch reviewer.") so the
workflow doesn't proceed with absent/misformatted HIGH_LINES; reference the
CYCLE_SUMMARY, HIGH_COUNT and HIGH_LINES extraction/parsing logic and the "##
Current HIGH Concerns" section when implementing this check.
- Around line 134-136: Add clear, short definitions for the terms "PARTIALLY
RESOLVED" and "FULLY RESOLVED" next to the INCLUDE/EXCLUDE guidance so reviewers
count HIGHs consistently; specify that PARTIALLY RESOLVED means the HIGH is
acknowledged and a mitigation or fix is in progress but not yet
verified/completed (include expected evidence like an open ticket or remediation
plan), and that FULLY RESOLVED means the issue is addressed and
verification/closure steps are complete (include evidence such as closed ticket,
verification log, or reviewer sign-off). Reference the existing INCLUDE list
wording ("PARTIALLY RESOLVED HIGHs" and "explicitly RESOLVED/FULLY RESOLVED
HIGHs") and insert the two one-line criteria immediately after that list.
- Line 163: The parsing of HIGH_COUNT using the regex
`CYCLE_SUMMARY:\s+current_high=(\d+)` should distinguish between a missing
CYCLE_SUMMARY and a malformed value; update the code that currently aborts with
"Review agent did not honor the CYCLE_SUMMARY contract — cannot determine HIGH
count. Retry or switch reviewer." to first check for a missing match and keep
the existing abort message, and if there is a match but the captured value does
not parse to an integer (or fails validation), abort with a different, explicit
message like "CYCLE_SUMMARY present but current_high is malformed — expected
integer, got '<value>'", referencing the `HIGH_COUNT` extraction logic so
callers can debug formatting vs absence.

Adversarial review — requesting changes

This is a cross-AI plan convergence loop with replanning. I'm evaluating the iteration on top of #2306 as shipped, plus what this PR actually changes (160/72 touching workflow + tests; CYCLE_SUMMARY contract, --ws symmetry, behavioral tests, slash-prefix rename).

Gall's Law — was this evolved from something simpler?

Yes, and visibly. Commit history shows this started as grep-of-REVIEWS.md, then trek-e's review + a latent cycle-awareness bug drove the move to a CYCLE_SUMMARY return contract. That is a legitimate evolutionary step from a working simpler system; credit where due. But the system is now simultaneously (a) a shell orchestrator writing $HIGH_COUNT-style pseudocode and (b) an LLM-level parsing layer — and the PR dropped the "Parsing layer:" note in d6b1a94 on the grounds that "surrounding text already implies LLM parsing." It does not imply that for anyone reading HIGH_COUNT=...regex match... at get-shit-done/workflows/plan-review-convergence.md:162-164, which is written as if something executes it. The note was not appeasement; the note was the one line telling a future implementer not to try to wire bash around this. Put it back.

Goodhart's Law — the termination metric is the vulnerability

"Loop until HIGH_COUNT == 0 or MAX_CYCLES" with HIGH_COUNT reported by the reviewer itself creates a target-becomes-measure: the reviewer learns (or is coached by the replanner) that downgrading HIGHs to MEDIUMs ends the loop. The CYCLE_SUMMARY contract at plan-review-convergence.md:125-142 is an honor-system instruction to an external AI — Codex/Gemini/Copilot — that has no skin in distinguishing HIGH from MEDIUM accurately. The "EXCLUDE: explicitly RESOLVED HIGHs" rule at :134 is especially exploitable: a replan that renames a concern rather than fixing it satisfies the contract. There is no guard against severity drift across cycles (e.g., tracking that no previously-HIGH became MEDIUM without a RESOLVED marker). Add one, or acknowledge this in the workflow as a known failure mode rather than shipping the metric as reliable.

Peter Principle — unbounded adversarial reviewer

If the external reviewer keeps inventing new HIGHs each cycle (not the "stall" case — the "invents different HIGHs each time" case), prev_high_count comparison at :170 only catches non-decreasing counts. 5 fresh HIGHs → 5 different fresh HIGHs reads as stall; 5 → 4 → 3 cascading churn of different concerns reads as progress and exhausts all 3 cycles. There is no "churn detection" (HIGH identity tracking across cycles). MAX_CYCLES caps wall time but not reviewer adversariality. Acceptable for this PR if called out — currently it is not.

Kernighan — state machine

Control flow is fine: 5a → 5b → 5c → 5d → 5a is linear. No clever async. prev_high_count is initialized before the loop (confirmed :170 reference), so the first-cycle stall-false-positive case is handled. This part is boring in the good way.

Knuth — cost per cycle unmeasured

3 cycles × (1 plan-phase + 1 cross-AI review + 1 replan) = ~9 LLM-agent invocations worst case, each themselves spawning sub-skills. No measurement, no budget, no --max-tokens or --max-cost escape hatch. For an XL feature whose name is "convergence loop" this is the first question an operator will ask. At minimum, log per-cycle timing so users can see what they paid.

Minor

• plan-review-convergence.md:249 — success_criteria still says "grep HIGHs". e38f66d was supposed to sync this. Verify: the diff shows it was updated to "parse CYCLE_SUMMARY for HIGH count", but the file at HEAD above still reads "grep HIGHs" at line 249. Check this is actually committed.
• CYCLE_SUMMARY contract failure mode at :144 is fail-loud ("abort"). Good. But there is no retry with a different reviewer — one malformed return ends the whole loop. Consider one retry before abort.
• tests/plan-review-convergence.test.cjs — 39 tests are all content assertions on workflow strings. These verify the prompt text exists, not that the LLM actually honors it. That is the limit of the pattern, but claiming "BEHAVIORAL" in test names (e.g. BEHAVIORAL: decreasing HIGH count...) is overclaim: those tests simulate a JS function stallCondition, not the workflow. Rename to SIMULATED: or drop the caps.

Verdict

Changes requested. Core loop evolution is sound; the CYCLE_SUMMARY migration is a real improvement over raw grep. Blockers: (1) success_criteria line 249 still says grep, (2) no churn detection / severity-drift guard against the Goodhart attack the loop invites, (3) no cost observability for a loop that costs N×3 LLM calls, (4) restore the dropped Parsing-layer note. Address these, then this ships.